Factory for SAXParser parsing XML without DOCTYPE #632
Conversation
Test Results 252 files + 3 252 suites +3 57m 2s ⏱️ - 10m 52s For more details on these failures and errors, see this check. Results for commit 377b2a4. ± Comparison against base commit e5217a8. ♻️ This comment has been updated with latest results. |
jukzi
force-pushed
the
xmlparser
branch
3 times, most recently
from
1b3a7d9
to
c3eaa6e
Compare
|
random failing ApiDescriptionProcessorTests #647 |
External entity resolution is not supported by PDE (see PDECoreMessages.XMLErrorReporter_ExternalEntityResolution) but still the SAXParser did follow external links where DefaultHandler.resolveEntity was not overwritten. At many places PDE already overwrote DefaultHandler.resolveEntity to prevent external resolution. With the new configuration that method is not even called anymore. This change offers and uses a configuration that * reports an Exception if .xml contains DOCTYPE or * does just ignore external references (as a fall back if the exception would show up to cause trouble). Also the caching of used parsers in possibly other threads is removed because the SAXParser is not guaranteed to be thread-safe. Only the factory is reused, because that is effectively final after creation. Reusing SAXParser is not a big help nowadays - see XmlParserFactoryTest.main(String[]) for performance test. In my measurement successive parser creations takes only ~ 0.06 ms.
|
ignoring random failing tests |
|
Nice job on this and the other PRs regarding XML-parser creation. Since the |
| } | ||
| assertTrue(firstLine, firstLine.startsWith("GET")); | ||
| assertFalse("Server received secret: " + firstLine, firstLine.contains("secret")); // var3 | ||
| assertFalse("Server was contacted", true); |
There was a problem hiding this comment.
Looks like this is always throwing an exception.
Can you tell why the test passes nevertheless?
There was a problem hiding this comment.
One test expects an exception (but does not come so far because of another exception) while another test makes sure the code is not called at all.
|
External entity resolution is not supported by PDE (see PDECoreMessages.XMLErrorReporter_ExternalEntityResolution) but still the SAXParser did follow external links where DefaultHandler.resolveEntity was not overwritten.
At many places PDE already overwrote DefaultHandler.resolveEntity to prevent external resolution. With the new configuration that method is not even called anymore.
This change offers and uses a configuration that
Also the caching of used parsers in possibly other threads is removed because the SAXParser is not guaranteed to be thread-safe. Only the factory is reused, because that is effectively final after creation. Reusing SAXParser is not a big help nowadays - see XmlParserFactoryTest.main(String[]) for performance test. In my measurement successive parser creations takes only ~ 0.06 ms.